SANDBOX-1561 | feature: Make targets for debugging services and operators #1242
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MikelAlejoBR The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds a local E2E debug deployment flow: a new Make target exports DEBUG_MODE=true, builds/pushes debug-tagged images, patches registration service and operator CSVs to run with Delve (debugger on port 50000), and reduces replicas to enable IDE port-forwarding. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (IDE)
participant Make as Makefile (`dev-deploy-e2e-local-debug`)
participant CI as CI scripts (`manage-*.sh`)
participant Builder as Image builder (podman/docker)
participant OC as OpenShift (oc/kubectl)
participant Pod as Operator Pod (with Delve)
Dev->>Make: run `dev-deploy-e2e-local-debug`
Make->>CI: invoke `manage-*-operator.sh` with DEBUG_MODE
CI->>CI: push_image(DEBUG_MODE) → determine DEBUG_MODE_SUFFIX
CI->>Builder: call `${IMAGE_BUILDER}-push${DEBUG_MODE_SUFFIX}`
Builder->>CI: image pushed
CI->>OC: apply manifests / update images
Make->>OC: patch registration/CSV to use Delve command/args and reduce replicas
OC->>Pod: operator pods start with Delve (listen :50000)
Dev->>Pod: port-forward 50000 → connect debugger
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/ci/manage-operator.sh (1)
47-62: Good implementation of debug mode support.The conditional logic correctly enables debug-mode image builds by appending
-debugto the make target whenDEBUG_MODEis"true". This allows the downstream make targets to build Delve-enabled images.Optional: standardize indentation
The indentation within the if-else block is inconsistent. Consider aligning for better readability:
push_image() { # When the "${DEBUG_MODE}" argument is passed, we instruct Make to push # the "debug" images with Delve on them. if [[ $1 == "true" ]]; then - DEBUG_MODE_SUFFIX="-debug" - else - DEBUG_MODE_SUFFIX="" + DEBUG_MODE_SUFFIX="-debug" + else + DEBUG_MODE_SUFFIX="" fimake/dev.mk (2)
54-55: Consider renaming CVS to CSV for clarity.The variable names use "CVS" but the resource type is "ClusterServiceVersion," commonly abbreviated as "CSV." Using "CSV" would be clearer and avoid confusion with the legacy version control system.
Suggested naming improvement
# Get the CVSs for the host and member operators, in order to be able to # patch them. - HOST_CVS_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion) - MEMBER_CVS_NAME=$$(oc get --namespace ${DEFAULT_MEMBER_NS} --output name ClusterServiceVersion) + HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion) + MEMBER_CSV_NAME=$$(oc get --namespace ${DEFAULT_MEMBER_NS} --output name ClusterServiceVersion)Then update references on lines 61, 69, and 70 accordingly.
69-70: Consider improving readability of long patch commands.The JSON patch commands are very long (200+ characters), making them difficult to review and maintain. While the logic appears correct, consider breaking them into variables or using here-documents for better readability.
Example: Extract patch JSON to variables
+ # Define patch for host operator + HOST_PATCH='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/host-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' + + # Define patch for member operator + MEMBER_PATCH='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/member-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' + # Patch the host-operator and member-operator CSVs to make them run with # Delve. - oc patch --namespace "${DEFAULT_HOST_NS}" "${HOST_CVS_NAME}" --type='json' --patch='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/host-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' - oc patch --namespace "${DEFAULT_MEMBER_NS}" "${MEMBER_CVS_NAME}" --type='json' --patch='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/member-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' + oc patch --namespace "${DEFAULT_HOST_NS}" "${HOST_CVS_NAME}" --type='json' --patch="$${HOST_PATCH}" + oc patch --namespace "${DEFAULT_MEMBER_NS}" "${MEMBER_CVS_NAME}" --type='json' --patch="$${MEMBER_PATCH}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.adocmake/dev.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.
Applied to files:
make/dev.mkREADME.adoc
🧬 Code graph analysis (2)
scripts/ci/manage-host-operator.sh (1)
scripts/ci/manage-operator.sh (1)
push_image(47-62)
scripts/ci/manage-member-operator.sh (1)
scripts/ci/manage-operator.sh (1)
push_image(47-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: Unit Tests
🔇 Additional comments (5)
scripts/ci/manage-host-operator.sh (1)
99-99: LGTM! DEBUG_MODE propagation is correct.The
push_imagecalls now correctly propagate theDEBUG_MODEenvironment variable to enable debug-enabled image builds when set. The variable is expected to be exported by the caller (e.g.,make/dev.mk), and if unset, the function will default to the standard non-debug build path.Also applies to: 111-111
scripts/ci/manage-member-operator.sh (1)
98-98: LGTM! Consistent with host operator changes.The
push_imageinvocation correctly propagatesDEBUG_MODEto enable debug builds, matching the pattern established inmanage-host-operator.sh.README.adoc (1)
154-161: Excellent documentation for the debug target.The documentation clearly explains the new debugging workflow, including:
- Build characteristics (no optimizations, Delve included)
- Debugger port (50000)
- Port-forwarding instructions
- List of debuggable services
This will help developers quickly understand and use the debugging feature.
make/dev.mk (2)
42-50: Good use of export for DEBUG_MODE propagation.The target correctly exports
DEBUG_MODE=trueto enable debug builds in downstream make targets and shell scripts. The.ONESHELL:directive appropriately enables multi-line shell commands for the complex patching logic that follows.
61-61: Verify: Silent failure handling may hide errors.The
|| truesilently ignores failures when patching the registration service command. If the CSV doesn't exist or the patch path is invalid, this will fail silently and the registration service won't be configured for debugging.Consider whether this should fail loudly or if the silent handling is intentional (e.g., for cases where the CSV structure differs).
Would it be better to check if the patch succeeded and provide a warning, or is silent failure acceptable here?
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
bf2dec3 to
b390515
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
make/dev.mk (1)
54-55: Verify CSV retrieval when multiple CSVs exist.The command
oc get --output name ClusterServiceVersionreturns all CSVs in the namespace. If there are multiple CSVs (e.g., from previous deployments or other operators), the variable will contain multiple names, potentially causing the subsequentoc patchcommands to fail or behave unexpectedly.Consider filtering for the specific operator CSV:
Suggested improvement
- HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion) - MEMBER_CSV_NAME=$$(oc get --namespace ${DEFAULT_MEMBER_NS} --output name ClusterServiceVersion) + HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion | grep toolchain-host-operator) + MEMBER_CSV_NAME=$$(oc get --namespace "${DEFAULT_MEMBER_NS}" --output name ClusterServiceVersion | grep toolchain-member-operator)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.adocmake/dev.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci/manage-host-operator.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.
Applied to files:
make/dev.mkREADME.adoc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: GolangCI Lint
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (4)
scripts/ci/manage-member-operator.sh (1)
98-98: LGTM!The change correctly passes
DEBUG_MODEtopush_image, aligning with the pattern inmanage-host-operator.sh. WhenDEBUG_MODEis unset or empty,push_imagewill default to the non-debug push target, which is the expected behavior.README.adoc (1)
154-160: LGTM!The documentation clearly explains the new
dev-deploy-e2e-local-debugtarget, including its purpose, how to connect (port-forwarding on 50000), and which services are debuggable. This aligns with the implementation inmake/dev.mk.make/dev.mk (2)
69-70: Hardcoded container indices may be fragile.The patches use fixed indices (
containers/1for host-operator,containers/0for member-operator). If the container order in the CSV deployment spec changes, the wrong container will be patched.This is acceptable for a dev-only debugging target, but be aware that changes to the operator CSV structures could silently break this functionality.
42-50: Good use of.ONESHELLand exported variable.The
.ONESHELLdirective correctly ensures shell variables persist across recipe lines, andexport DEBUG_MODE=trueproperly propagates the flag to downstream Make invocations viadev-deploy-e2e-local.
|
|
||
| if [[ ${PUBLISH_OPERATOR} == "true" ]]; then | ||
| push_image | ||
| push_image "${DEBUG_MODE}" |
There was a problem hiding this comment.
Wonder whether this should be added as an option for consistency with other options or it's fine to treat it as a special environment variable.
@MatousJobanek and @rsoaresd may have thoughts on this
There was a problem hiding this comment.
You are right, yes. I do agree that it's better to have an additional option for consistency. Refactored!
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
b390515 to
e4931d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @make/dev.mk:
- Line 63: Remove the silent-failure suppression on the registration-service
patch by deleting the trailing "|| true" from the oc patch that adds
REGISTRATION_SERVICE_COMMAND to the HOST_CSV_NAME container so failures surface;
make error handling consistent across the other oc patch invocations (the
ToolchainConfig and operator CSV patches) by ensuring they also do not use "||
true" (or explicitly handle the specific "already exists" condition), and if
needed implement an explicit check for the env entry before patching instead of
swallowing all errors.
- Line 50: Uncomment the deployment invocation so the dev target actually
deploys e2e resources: remove the leading '#' from the "$(MAKE)
dev-deploy-e2e-local" line in make/dev.mk so the target runs the deployment step
(ensuring subsequent "oc get ClusterServiceVersion" and patch commands have
resources to operate on and match the README.adoc behavior).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.adocmake/dev.mkmake/test.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/ci/manage-host-operator.sh
- scripts/ci/manage-member-operator.sh
- scripts/ci/manage-operator.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.
Applied to files:
make/test.mkmake/dev.mkREADME.adoc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (3)
make/test.mk (2)
334-338: LGTM! Consistent parameter handling.The DEBUG_MODE_PARAM evaluation follows the established pattern used for other conditional parameters in this file (FORCED_TAG_PARAM, MEMBER_NS_2_PARAM, etc.). The double-negation checks ensure the parameter is only set when DEBUG_MODE has a non-empty value.
348-348: LGTM! Debug mode correctly integrated into non-latest deployment paths.The DEBUG_MODE_PARAM is appropriately passed only in the non-DEPLOY_LATEST branches, which makes sense since debug mode requires building custom images with Delve rather than using pre-published latest images.
Also applies to: 378-378
README.adoc (1)
154-161: Documentation is clear and informative.The documentation effectively describes the new debugging target, including the port-forwarding workflow and which services are built with Delve support. The explanation will help developers set up their debugging environment.
| # then an IDE can be connected to them. Since the targets down the line use | ||
| # the default namespaces, we can use them to patch the required CRs in order | ||
| # to launch the binaries with Delve. | ||
| .ONESHELL: |
There was a problem hiding this comment.
.ONESHELL is a global directive that affects all subsequent targets.
The .ONESHELL directive on Line 46 applies to all targets defined after it in this file, not just dev-deploy-e2e-local-debug. This can have unintended consequences:
- All subsequent targets will run their recipes in a single shell, which changes error propagation behavior.
- Other targets may not expect this behavior and could mask failures.
If .ONESHELL is needed only for variable persistence in the debug target (for HOST_CSV_NAME and MEMBER_CSV_NAME), consider moving it to a separate included file or restructuring the target to avoid the need for .ONESHELL.
♻️ Alternative approach
Instead of .ONESHELL, you could capture variables and pass them to a sub-shell:
-.ONESHELL:
.PHONY: dev-deploy-e2e-local-debug
dev-deploy-e2e-local-debug: export DEBUG_MODE=true
dev-deploy-e2e-local-debug:
- # $(MAKE) dev-deploy-e2e-local
-
- # Get the CSVs for the host and member operators, in order to be able to
- # patch them.
- HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion)
- MEMBER_CSV_NAME=$$(oc get --namespace ${DEFAULT_MEMBER_NS} --output name ClusterServiceVersion)
-
- @echo "CVSs are: $${HOST_CSV_NAME} and $${MEMBER_CSV_NAME}"
- ...
+ $(MAKE) dev-deploy-e2e-local
+ @bash -c ' \
+ set -e; \
+ HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion); \
+ MEMBER_CSV_NAME=$$(oc get --namespace "${DEFAULT_MEMBER_NS}" --output name ClusterServiceVersion); \
+ echo "CSVs are: $$HOST_CSV_NAME and $$MEMBER_CSV_NAME"; \
+ ...'This limits the .ONESHELL-like behavior to only this target.
Committable suggestion skipped: line range outside the PR's diff.
Adds a new Make target which triggers build in the underlying services and operators which end up building the images without any code optimizations and with Delve on them. Once pushed and deployed to the local OpenShift cluster, it patches the host operator, the member operator and the registration service to launch them with the Delve executable, which allows debugging them on port 50000 after port-forwarding to those pods. SANDBOX-1561
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
e4931d6 to
5cbf02f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
make/dev.mk (2)
46-46:.ONESHELLaffects all subsequent targets in this file.The
.ONESHELLdirective on line 46 applies globally to all targets defined after it in this file, not justdev-deploy-e2e-local-debug. This changes how Make executes recipes for all subsequent targets (they run in a single shell instead of one shell per line), which can alter error propagation and variable behavior.While
.ONESHELLis needed here for variable persistence (HOST_CSV_NAMEandMEMBER_CSV_NAME), consider either:
- Moving this target to a separate included file with its own
.ONESHELLscope- Documenting this behavior clearly for future maintainers
- Restructuring to avoid
.ONESHELL(e.g., using a bash script with-c)Based on learnings from metlos, Make recipe lines starting with
#are printed before execution, but this directive affects more fundamental execution behavior.
69-72: Consider extracting JSON patches to files for readability.The inline JSON patch strings are quite long and difficult to review. While functionally correct, consider extracting them to separate JSON files and using
--patch-filefor improved maintainability and readability.Example refactor approach
Create
patches/host-operator-delve-patch.jsonandpatches/member-operator-delve-patch.json, then reference them:- oc patch --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --type='json' --patch='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/host-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' + oc patch --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --type='json' --patch-file=patches/host-operator-delve-patch.json
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.adocmake/dev.mkmake/test.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- README.adoc
- scripts/ci/manage-member-operator.sh
- scripts/ci/manage-host-operator.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.
Applied to files:
make/dev.mkscripts/ci/manage-operator.shmake/test.mk
📚 Learning: 2025-08-28T12:28:22.801Z
Learnt from: metlos
Repo: codeready-toolchain/toolchain-e2e PR: 1187
File: make/clean.mk:6-11
Timestamp: 2025-08-28T12:28:22.801Z
Learning: In make, recipe lines that start with `#` are printed to stdout before being passed to the shell, making them visible as output even though they're shell comments. To suppress this output, the line would need to be prefixed with `@#`.
Applied to files:
make/dev.mk
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: GolangCI Lint
- GitHub Check: Unit Tests
🔇 Additional comments (8)
scripts/ci/manage-operator.sh (3)
48-54: LGTM! Clean DEBUG_MODE handling.The conditional logic correctly sets the DEBUG_MODE_SUFFIX based on the first argument, with clear explanatory comments.
56-57: LGTM! Sensible default.Setting a default IMAGE_BUILDER ensures the variable is always defined.
47-65: This feature has already been merged and deployed as of December 23, 2025. The commit "feature: Make targets for debugging services and operators" (5cbf02f) includes the modifiedpush_image()function and is documented in README.adoc as a working feature (make dev-deploy-e2e-local-debug). No blocking work or external dependencies remain.make/dev.mk (3)
50-50: LGTM! Deployment invocation is now active.The deployment step is correctly uncommented, fixing the critical issue flagged in previous reviews where this line was commented out and would cause subsequent
oc getcommands to fail.
52-55: LGTM! CSV retrieval logic is correct.The commands correctly fetch the CSV names for both operators using
oc get --output name, storing them in shell variables that persist across subsequent recipe lines due to.ONESHELL.
57-67: LGTM! Improved error handling with conditional check.The registration service patch now uses a conditional check (lines 61-63) to verify the environment variable doesn't already exist before patching, which is much better than the
|| truepattern flagged in previous reviews. This provides explicit error handling while avoiding duplicate patches.The ToolchainConfig patch (line 67) correctly fails loudly if it encounters errors.
make/test.mk (2)
368-378: LGTM! Consistent parameter pattern.The DEBUG_MODE_PARAM handling mirrors the existing FORCED_TAG_PARAM pattern and correctly passes the flag only in non-DEPLOY_LATEST flows. The
-dmflag is supported in manage-host-operator.sh.
334-348: LGTM! Consistent parameter pattern.The DEBUG_MODE_PARAM handling mirrors the existing FORCED_TAG_PARAM pattern and correctly passes the flag only in non-DEPLOY_LATEST flows.
|
3 participants
Adds a new Make target which triggers build in the underlying services and operators which end up building the images without any code optimizations and with Delve on them.
Once pushed and deployed to the local OpenShift cluster, it patches the host operator, the member operator and the registration service to launch them with the Delve executable, which allows debugging them on port 50000 after port-forwarding to those pods.
Related PRs
Jira ticket
[SANDBOX-1561]
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.